Repository navigation
fix(engine): order keyed NotInParallel tests by priority, not hash code - #7090
Conversation
TestGroupingService passed TestPriority.GetHashCode() to ConstraintKeyScheduler as the sort key for keyed [NotInParallel] tests. The record struct hash does not preserve ordering: with the default priority, any Order above ~894M (including the default Order) wraps negative, and different ExecutionPriority values land in arbitrary positions. Pass a dense rank derived from TestPriority's ordering instead, for both the top-level keyed list and constrained parallel groups. Fixes #7088
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 5 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Medium impact] The PR appears safe to merge. SummaryReplaces hash-based sort keys with ordered ranks for keyed
Reviews (1) · Last reviewed commit: "fix(engine): order keyed NotInParallel t..." · Reviewed by Greptile |
ReviewThe fix is correct and well-scoped. I found no blocking issues.
Two non-blocking notes:
LGTM. |
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.75.0 to 1.76.28. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.76.28 <!-- Release notes generated using configuration in .github/release.yml at v1.76.28 --> ## What's Changed ### Other Changes * Remove legacy JSON output implementation by @Saad-elnagar in thomhurst/TUnit#7234 * fix(engine): drop row-scoped data sources from failed test metadata by @kenny-ish in thomhurst/TUnit#7079 * Merge the analyzer release tables under one New Rules section (RS2007) by @bfs2021 in thomhurst/TUnit#7174 * fix: release finished test instances before session end by @thomhurst in thomhurst/TUnit#7262 * fix: never hand out a disposed shared data-source instance by @thomhurst in thomhurst/TUnit#7248 ### Dependencies * chore(deps): update all non-major dependencies to 1.76.0 by @thomhurst in thomhurst/TUnit#7233 * chore(deps): bump follow-redirects from 1.16.0 to 1.16.1 in /docs by @dependabot[bot] in thomhurst/TUnit#7232 * chore(deps): bump launch-editor from 2.14.1 to 2.14.2 in /docs by @dependabot[bot] in thomhurst/TUnit#7231 * chore(deps): update all non-major dependencies to 1.20.1 by @thomhurst in thomhurst/TUnit#7235 * chore(deps): update dependency awssdk.sqs to 4.0.100.20 by @thomhurst in thomhurst/TUnit#7236 * chore(deps): update dependency microsoft.playwright to 1.64.0 by @thomhurst in thomhurst/TUnit#7237 * chore(deps): update all non-major dependencies by @thomhurst in thomhurst/TUnit#7238 * chore(deps): update dependency path-to-regexp to v9 by @thomhurst in thomhurst/TUnit#7241 * chore(deps): update all non-major dependencies by @thomhurst in thomhurst/TUnit#7242 * chore(deps): update dependency serialize-javascript to v7.1.3 by @thomhurst in thomhurst/TUnit#7243 * chore(deps): update dependency imposter to 1.0.4 by @thomhurst in thomhurst/TUnit#7244 * chore(deps): update dependency stackexchange.redis to 3.4.20 by @thomhurst in thomhurst/TUnit#7245 * chore(deps): update dependency serialize-javascript to v7.1.4 by @thomhurst in thomhurst/TUnit#7246 * chore(deps): update dependency imposter to 1.0.5 by @thomhurst in thomhurst/TUnit#7247 * chore(deps): update all non-major dependencies to 13.6.4 by @thomhurst in thomhurst/TUnit#7251 * chore(deps): update dependency imposter to 1.0.6 by @thomhurst in thomhurst/TUnit#7258 * chore(deps): bump fast-uri from 3.1.8 to 3.1.9 in /docs by @dependabot[bot] in thomhurst/TUnit#7253 * chore(deps): bump body-parser from 1.20.6 to 1.20.9 in /docs by @dependabot[bot] in thomhurst/TUnit#7252 ## New Contributors * @Saad-elnagar made their first contribution in thomhurst/TUnit#7234 * @kenny-ish made their first contribution in thomhurst/TUnit#7079 * @bfs2021 made their first contribution in thomhurst/TUnit#7174 **Full Changelog**: thomhurst/TUnit@v1.76.0...v1.76.28 ## 1.76.0 <!-- Release notes generated using configuration in .github/release.yml at v1.76.0 --> ## What's Changed ### Other Changes * chore: add OSS scanner config, threat model and SECURITY.md by @thomhurst in thomhurst/TUnit#7080 * fix(deps): patch MessagePack for Aspire and use per-TFM Microsoft.Extensions.Http by @Quattordici in thomhurst/TUnit#7072 * fix: skip tests marked with an empty skip reason by @thomhurst in thomhurst/TUnit#7083 * fix(engine): report the engine mode actually selected in the banner by @thomhurst in thomhurst/TUnit#7085 * fix(engine): order keyed NotInParallel tests by priority, not hash code by @thomhurst in thomhurst/TUnit#7090 * fix: remove root causes of flaky tests and the broken Apple smoke jobs by @thomhurst in thomhurst/TUnit#7095 * fix(engine): run synchronous keyed NotInParallel tests concurrently by @thomhurst in thomhurst/TUnit#7087 * fix(sourcegen): run every [Arguments] row that infers the same generic type by @thomhurst in thomhurst/TUnit#7100 * fix: same filter path and display name for generic tests in both engine modes by @thomhurst in thomhurst/TUnit#7110 * fix(engine): apply max parallelism cap to keyed NotInParallel tests by @thomhurst in thomhurst/TUnit#7084 * fix(ci): install only Chromium for Playwright and wait on dpkg lock by @thomhurst in thomhurst/TUnit#6994 * fix(core): keep the engine-selected execution mode when late assemblies load by @thomhurst in thomhurst/TUnit#7091 * fix(engine): end fail-fast runs without crashing the test host by @thomhurst in thomhurst/TUnit#7115 * test: stop the AOT [EngineTest=Pass] run hitting its 5 minute budget by @thomhurst in thomhurst/TUnit#7125 * fix(engine): align reflection-mode data sources and hooks with source-gen by @thomhurst in thomhurst/TUnit#7094 * fix(pipeline): stop skipping the NuGet package tester modules by @thomhurst in thomhurst/TUnit#7127 * ci: take AOT publishes and doc snippets off the Ubuntu critical path by @thomhurst in thomhurst/TUnit#7128 * fix(analyzers): TUnit0001 false positive for MethodDataSource on generic type-parameter parameters by @thomhurst in thomhurst/TUnit#7109 * fix: report generic class constraint violations as failed tests in both modes by @thomhurst in thomhurst/TUnit#7112 * ci: cut runner contention and the AOT publish off the critical path by @thomhurst in thomhurst/TUnit#7131 * fix(assertions): make cross-type IsEqualTo work for numeric and nullable arguments on C# 12 by @thomhurst in thomhurst/TUnit#7086 * fix(engine): substitute class type arguments and array element types in build-time constraint checks by @thomhurst in thomhurst/TUnit#7102 * ci: test the NuGet packages on Windows in their own job by @thomhurst in thomhurst/TUnit#7130 * fix(sourcegen): bind params T[] on generic test methods by @thomhurst in thomhurst/TUnit#7101 * fix(engine): run every [Arguments] row on generic test classes in both modes by @thomhurst in thomhurst/TUnit#7111 * fix(engine): use last inferred type for repeated generic parameters by @theluckystrike in thomhurst/TUnit#7134 * Make the test suite pass in every environment and run fully in CI by @Quattordici in thomhurst/TUnit#7129 * fix(assertions): skip using directive for global-namespace assertion types by @thomhurst in thomhurst/TUnit#7139 * fix(core): format numeric display-name arguments with the invariant culture by @thomhurst in thomhurst/TUnit#7138 * fix(engine): dispose data-source objects of rows that end as failed tests by @thomhurst in thomhurst/TUnit#7150 * Fix net472 compile gate failure in assertions source generator tests by @thomhurst in thomhurst/TUnit#7170 * fix(sourcegen): close generic methods of generic classes on every class source by @thomhurst in thomhurst/TUnit#7152 * fix(engine): infer object for null rows on generic test methods in reflection mode by @thomhurst in thomhurst/TUnit#7142 * fix(engine): report every test once on fail-fast runs by @thomhurst in thomhurst/TUnit#7145 * test(engine): stop whole-process budgets killing healthy scheduling engine tests by @thomhurst in thomhurst/TUnit#7177 * feat(analyzers): report MethodDataSource Arguments that fit two overloads equally (TUnit0076) by @thomhurst in thomhurst/TUnit#7181 * fix(sourcegen): render top-level dynamic as object so typeof compiles by @thomhurst in thomhurst/TUnit#7182 * fix(sourcegen): pair each GenerateGenericTest type only with its typed ClassDataSource<T> by @thomhurst in thomhurst/TUnit#7188 * fix(engine): run untyped [Arguments(null)] rows on generic classes instead of dropping them by @thomhurst in thomhurst/TUnit#7203 * fix(engine): report generic test classes by their generic type definition in TRX by @thomhurst in thomhurst/TUnit#7200 * test(sourcegen): parse snapshot fixtures as a real build does by @thomhurst in thomhurst/TUnit#7189 * refactor(engine): dedupe reflection generic discovery by @thomhurst in thomhurst/TUnit#7183 * test: make TimeoutHelperTests deterministic under thread-pool load by @thomhurst in thomhurst/TUnit#7180 * fix(analyzers): accept null MethodDataSource arguments for nullable parameters by @thomhurst in thomhurst/TUnit#7179 * test(assertions): make WaitsFor cancellation tests robust to slow CI runners by @thomhurst in thomhurst/TUnit#7176 * fix(assertions): support [AssertionFrom] with unbound generic types by @thomhurst in thomhurst/TUnit#7144 ... (truncated) Commits viewable in [compare view](thomhurst/TUnit@v1.75.0...v1.76.28). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.75.0 to 1.76.28. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.76.28 <!-- Release notes generated using configuration in .github/release.yml at v1.76.28 --> ## What's Changed ### Other Changes * Remove legacy JSON output implementation by @Saad-elnagar in thomhurst/TUnit#7234 * fix(engine): drop row-scoped data sources from failed test metadata by @kenny-ish in thomhurst/TUnit#7079 * Merge the analyzer release tables under one New Rules section (RS2007) by @bfs2021 in thomhurst/TUnit#7174 * fix: release finished test instances before session end by @thomhurst in thomhurst/TUnit#7262 * fix: never hand out a disposed shared data-source instance by @thomhurst in thomhurst/TUnit#7248 ### Dependencies * chore(deps): update all non-major dependencies to 1.76.0 by @thomhurst in thomhurst/TUnit#7233 * chore(deps): bump follow-redirects from 1.16.0 to 1.16.1 in /docs by @dependabot[bot] in thomhurst/TUnit#7232 * chore(deps): bump launch-editor from 2.14.1 to 2.14.2 in /docs by @dependabot[bot] in thomhurst/TUnit#7231 * chore(deps): update all non-major dependencies to 1.20.1 by @thomhurst in thomhurst/TUnit#7235 * chore(deps): update dependency awssdk.sqs to 4.0.100.20 by @thomhurst in thomhurst/TUnit#7236 * chore(deps): update dependency microsoft.playwright to 1.64.0 by @thomhurst in thomhurst/TUnit#7237 * chore(deps): update all non-major dependencies by @thomhurst in thomhurst/TUnit#7238 * chore(deps): update dependency path-to-regexp to v9 by @thomhurst in thomhurst/TUnit#7241 * chore(deps): update all non-major dependencies by @thomhurst in thomhurst/TUnit#7242 * chore(deps): update dependency serialize-javascript to v7.1.3 by @thomhurst in thomhurst/TUnit#7243 * chore(deps): update dependency imposter to 1.0.4 by @thomhurst in thomhurst/TUnit#7244 * chore(deps): update dependency stackexchange.redis to 3.4.20 by @thomhurst in thomhurst/TUnit#7245 * chore(deps): update dependency serialize-javascript to v7.1.4 by @thomhurst in thomhurst/TUnit#7246 * chore(deps): update dependency imposter to 1.0.5 by @thomhurst in thomhurst/TUnit#7247 * chore(deps): update all non-major dependencies to 13.6.4 by @thomhurst in thomhurst/TUnit#7251 * chore(deps): update dependency imposter to 1.0.6 by @thomhurst in thomhurst/TUnit#7258 * chore(deps): bump fast-uri from 3.1.8 to 3.1.9 in /docs by @dependabot[bot] in thomhurst/TUnit#7253 * chore(deps): bump body-parser from 1.20.6 to 1.20.9 in /docs by @dependabot[bot] in thomhurst/TUnit#7252 ## New Contributors * @Saad-elnagar made their first contribution in thomhurst/TUnit#7234 * @kenny-ish made their first contribution in thomhurst/TUnit#7079 * @bfs2021 made their first contribution in thomhurst/TUnit#7174 **Full Changelog**: thomhurst/TUnit@v1.76.0...v1.76.28 ## 1.76.0 <!-- Release notes generated using configuration in .github/release.yml at v1.76.0 --> ## What's Changed ### Other Changes * chore: add OSS scanner config, threat model and SECURITY.md by @thomhurst in thomhurst/TUnit#7080 * fix(deps): patch MessagePack for Aspire and use per-TFM Microsoft.Extensions.Http by @Quattordici in thomhurst/TUnit#7072 * fix: skip tests marked with an empty skip reason by @thomhurst in thomhurst/TUnit#7083 * fix(engine): report the engine mode actually selected in the banner by @thomhurst in thomhurst/TUnit#7085 * fix(engine): order keyed NotInParallel tests by priority, not hash code by @thomhurst in thomhurst/TUnit#7090 * fix: remove root causes of flaky tests and the broken Apple smoke jobs by @thomhurst in thomhurst/TUnit#7095 * fix(engine): run synchronous keyed NotInParallel tests concurrently by @thomhurst in thomhurst/TUnit#7087 * fix(sourcegen): run every [Arguments] row that infers the same generic type by @thomhurst in thomhurst/TUnit#7100 * fix: same filter path and display name for generic tests in both engine modes by @thomhurst in thomhurst/TUnit#7110 * fix(engine): apply max parallelism cap to keyed NotInParallel tests by @thomhurst in thomhurst/TUnit#7084 * fix(ci): install only Chromium for Playwright and wait on dpkg lock by @thomhurst in thomhurst/TUnit#6994 * fix(core): keep the engine-selected execution mode when late assemblies load by @thomhurst in thomhurst/TUnit#7091 * fix(engine): end fail-fast runs without crashing the test host by @thomhurst in thomhurst/TUnit#7115 * test: stop the AOT [EngineTest=Pass] run hitting its 5 minute budget by @thomhurst in thomhurst/TUnit#7125 * fix(engine): align reflection-mode data sources and hooks with source-gen by @thomhurst in thomhurst/TUnit#7094 * fix(pipeline): stop skipping the NuGet package tester modules by @thomhurst in thomhurst/TUnit#7127 * ci: take AOT publishes and doc snippets off the Ubuntu critical path by @thomhurst in thomhurst/TUnit#7128 * fix(analyzers): TUnit0001 false positive for MethodDataSource on generic type-parameter parameters by @thomhurst in thomhurst/TUnit#7109 * fix: report generic class constraint violations as failed tests in both modes by @thomhurst in thomhurst/TUnit#7112 * ci: cut runner contention and the AOT publish off the critical path by @thomhurst in thomhurst/TUnit#7131 * fix(assertions): make cross-type IsEqualTo work for numeric and nullable arguments on C# 12 by @thomhurst in thomhurst/TUnit#7086 * fix(engine): substitute class type arguments and array element types in build-time constraint checks by @thomhurst in thomhurst/TUnit#7102 * ci: test the NuGet packages on Windows in their own job by @thomhurst in thomhurst/TUnit#7130 * fix(sourcegen): bind params T[] on generic test methods by @thomhurst in thomhurst/TUnit#7101 * fix(engine): run every [Arguments] row on generic test classes in both modes by @thomhurst in thomhurst/TUnit#7111 * fix(engine): use last inferred type for repeated generic parameters by @theluckystrike in thomhurst/TUnit#7134 * Make the test suite pass in every environment and run fully in CI by @Quattordici in thomhurst/TUnit#7129 * fix(assertions): skip using directive for global-namespace assertion types by @thomhurst in thomhurst/TUnit#7139 * fix(core): format numeric display-name arguments with the invariant culture by @thomhurst in thomhurst/TUnit#7138 * fix(engine): dispose data-source objects of rows that end as failed tests by @thomhurst in thomhurst/TUnit#7150 * Fix net472 compile gate failure in assertions source generator tests by @thomhurst in thomhurst/TUnit#7170 * fix(sourcegen): close generic methods of generic classes on every class source by @thomhurst in thomhurst/TUnit#7152 * fix(engine): infer object for null rows on generic test methods in reflection mode by @thomhurst in thomhurst/TUnit#7142 * fix(engine): report every test once on fail-fast runs by @thomhurst in thomhurst/TUnit#7145 * test(engine): stop whole-process budgets killing healthy scheduling engine tests by @thomhurst in thomhurst/TUnit#7177 * feat(analyzers): report MethodDataSource Arguments that fit two overloads equally (TUnit0076) by @thomhurst in thomhurst/TUnit#7181 * fix(sourcegen): render top-level dynamic as object so typeof compiles by @thomhurst in thomhurst/TUnit#7182 * fix(sourcegen): pair each GenerateGenericTest type only with its typed ClassDataSource<T> by @thomhurst in thomhurst/TUnit#7188 * fix(engine): run untyped [Arguments(null)] rows on generic classes instead of dropping them by @thomhurst in thomhurst/TUnit#7203 * fix(engine): report generic test classes by their generic type definition in TRX by @thomhurst in thomhurst/TUnit#7200 * test(sourcegen): parse snapshot fixtures as a real build does by @thomhurst in thomhurst/TUnit#7189 * refactor(engine): dedupe reflection generic discovery by @thomhurst in thomhurst/TUnit#7183 * test: make TimeoutHelperTests deterministic under thread-pool load by @thomhurst in thomhurst/TUnit#7180 * fix(analyzers): accept null MethodDataSource arguments for nullable parameters by @thomhurst in thomhurst/TUnit#7179 * test(assertions): make WaitsFor cancellation tests robust to slow CI runners by @thomhurst in thomhurst/TUnit#7176 * fix(assertions): support [AssertionFrom] with unbound generic types by @thomhurst in thomhurst/TUnit#7144 ... (truncated) Commits viewable in [compare view](thomhurst/TUnit@v1.75.0...v1.76.28). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fixes #7088
Root cause
TestGroupingServicesorted keyed[NotInParallel]tests correctly. It then passedTestPriority.GetHashCode()toConstraintKeyScheduleras each test'sintsort key. The scheduler sorts on that key and also uses it to choose which waiting test starts when a key is released. The compiler-generatedrecord structhash is roughly(int)Priority * -1521134295 + Order, computed with wrap-around, so it does not preserve ordering:Normalpriority, anyOrderabove about 894M wraps to a negative number. That includes the defaultOrder(int.MaxValue / 2), so keyed tests without anOrderran before tests withOrder = 1.[ExecutionPriority]values landed in arbitrary positions.Both hash sites were affected: the top-level keyed list and the keyed list of each constrained parallel group (
[ParallelGroup]+ keyed[NotInParallel]).Fix
The change is confined to
TestGroupingService;ConstraintKeyScheduleris untouched. Both keyed lists are now sorted withTestPriority.CompareTo(highest priority first, then ascendingOrder), with class name as the tie-break. Each test then gets a dense rank as itsintkey instead of the hash: tests that compare equal on priority share a rank. Because the scheduler's sort is stable on the original index, it keeps this order. The change adds no allocations: one int counter per list, and the sort lambdas are nowstatic.For the common case (default priority, small
Ordervalues), the resulting run order is the same as before, because the hash happened to be monotonic there.Tests
tests/TUnit.TestProject/Bugs/7088/KeyedNotInParallelOrderTests.cscovers keyed tests at top level and inside a[ParallelGroup]. They mixOrder = 1/2/3,Order = 1_000_000_000, the defaultOrder, and an[ExecutionPriority(High)]test. Each test checks that the tests that started before it are exactly the expected prefix.Order1000000000, DefaultOrder, HighPriority_Order50, Order1, Order2, Order3.tests/TUnit.Engine.Tests/Scheduling/KeyedNotInParallelOrderTests.csruns the above through the engine harness. Reflection passes locally; the AOT cases are CI-only.NotInParallelOrderExecutionTests,NotInParallelMixedTests,*ConstraintKey*and*NotInParallel*. The only failure under*NotInParallel*is the intentionalNotInParallelWithDependsOnTests.Step1.TUnit.UnitTestspasses (526 passed, 3 skipped). TheTUnit.Engine.TestsfiltersScheduling/*and*NotInParallel*pass, with AOT cases skipped locally.Overlap with open PRs
#7084 and #7087 both edit
src/TUnit.Engine/Scheduling/ConstraintKeyScheduler.cs. This PR does not touch that file, so there is no textual conflict. The scheduler still treatsPriorityas "lower runs first", which now holds by construction.